Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "interact button" enums: C, BC, BCA #1228

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

Dragorn421
Copy link
Collaborator

@Dragorn421 Dragorn421 commented May 16, 2022

Add three "interact button" enums:

  • one for C buttons,
  • one for B + C buttons,
  • one for B + C buttons + A

and macros for converting between them (ft. bottle adventure)
see z64save.h

(to get confirmation of the order of c buttons left/down/right, look at pauseCtx->equipTargetCBtn, or D_80854388)

@Roman971
Copy link
Collaborator

Roman971 commented Jun 5, 2022

I'm not opposed to this idea and I have considered it myself many times in the past, but I still don't know if it would be an improvement overall considering the amount of iteration/computation being done with these indices. That plus the A button only being in buttonStatus, and the 1-off mapping with C button indexes.

One thing I'll say is that if we were to have this enum, names like SAVE_BTN_B would probably be good enough imo

@Dragorn421
Copy link
Collaborator Author

Dragorn421 commented Jun 7, 2022

Yeah so because there's no way to have a single "save button" enum, I'm now thinking there could be one enum per array basically

So maybe:

gSaveContext.equips.buttonItems[SAVE_EQUIPS_BUTTON_ITEM_(B / C_LEFT / C_DOWN / C_RIGHT)]
gSaveContext.equips.cButtonSlots[SAVE_EQUIPS_C_BUTTON_SLOT_(LEFT / DOWN / RIGHT)]
gSaveContext.buttonStatus[SAVE_BUTTON_STATUS_(B / C_LEFT / C_DOWN / C_RIGHT / A)]

?

@Dragorn421 Dragorn421 force-pushed the doc_button_indices_in_save_structs branch from a49bc09 to 1fbd18c Compare October 12, 2022 05:08
@Dragorn421 Dragorn421 changed the title Add SaveButtonIndex enum Add "interact button" enums: C, BC, BCA Oct 12, 2022
@Dragorn421 Dragorn421 marked this pull request as ready for review October 12, 2022 05:12
@Dragorn421
Copy link
Collaborator Author

I was inspired

@mzxrules
Copy link
Contributor

Can't say that it makes much sense to me to have InteractButtonBCA and InteractButtonBC exist as separate enumerations. The only thing I see that it does is it allows you define IBTN_BC_MAX / IBTN_BCA_MAX without coupling it to a specific value, but I don't feel it is essential to define things in this way simply to achieve this effect.

@Dragorn421
Copy link
Collaborator Author

well, where a IBTN_BC_ value is used, not all IBTN_BCA_ values can be used (i.e. not A)

All three enums overlap, IBTN_C_ is also contained in both others, it just happens the two others start the same

The purpose of this change is documentation and making the code easier to read, may as well not introduce confusion by conflating two enums that share values

The PR as it stands isn't my first and only attempt at documenting the button array indices, I've tried several things before settling on this solution, so I'm past the improve-by-tinkering phase and would need concrete suggestions to improve it

@mzxrules
Copy link
Contributor

mzxrules commented Jan 21, 2023

Still seems unnecessary and bloaty to split the one enumeration into two just to avoid using IBTN_A outside of gSaveContext.buttonStatus usages, especially given that the two enums are coupled anyway.

@Dragorn421
Copy link
Collaborator Author

IBTN_C_ is also coupled to the two others

Using IBTN_BCA_B instead of IBTN_BC_B would hint IBTN_BCA_A can be used there, when it would be an OOB access
IBTN_BCA_TO_BC(IBTN_BCA_B) was an option I considered to have a single enum and still document what of BC/C/BCA indices were expected, but it looked bad imo

And yeah it's definitely more bloaty to use names like IBTN_C_C_LEFT instead of 0, it's a whole 12 characters longer, but that's the point, being easier to understand at the cost of being less terse. It's a common tradeoff when documenting anything including one's own code

@Dragorn421
Copy link
Collaborator Author

Dragorn421 commented Jan 22, 2023

cool, that's nice to hear

I would need a recap of what we're arguing about then

@AngheloAlf
Copy link
Contributor

I think he wants only one enum instead of two because those two enums overlap.
Personally I think merging those two enums would make stuff confusing and error prone (OoB accesses and such), so two enums sounds good to me

@mzxrules
Copy link
Contributor

mzxrules commented Jan 23, 2023

To me, the purpose of an enum is to define an ordering to some set of things, and not necessarily to index an array, and so splitting the larger set into a smaller set just to support indexing different sized arrays just doesn't feel clean. In this case it just happens to feel reasonable to do because there isn't a whole lot of code dependent on the enums, the full set doesn't have a NONE value to cause oobs access, and the duplication isn't adding tremendous amounts of code.

In any case, I await to see your split of SCENE_ into at least 4 enums.

@Dragorn421
Copy link
Collaborator Author

In any case, I await to see your split of SCENE_ into at least 4 enums.

which splits? I can only think of the dungeons one

and yeah enums overlap. that's just how OoT was coded. For example I think we have like 10 enums that are contained in the ItemID enum (including like player item action, exchange item, item slots, sword/tunic/boots (and their player counterparts), upgrades, quest items)

So at this point I have given up on having everything properly shifting on its own. Unfortunately, I think you just have to learn the codebase and know what to change if you wanted to edit this stuff, as imo favouring that in decomp would be too big of a hit to readability

splitting the larger set into a smaller set just to support indexing different sized arrays just doesn't feel clean

yeah it isn't, cf
... RBA https://github.com/Dragorn421/oot/blob/6fe01efdc88149271244f6242154b119da2fd54c/src/code/z_parameter.c#L2060
... and BA https://github.com/Dragorn421/oot/blob/6fe01efdc88149271244f6242154b119da2fd54c/src/code/z_parameter.c#L1231

where IBTN_BC_TO_C(...) evaluates to -1 with a bottle item on B :)

again it's just how oot is coded

@fig02 fig02 added Needs discussion There is a debate or other required discussion preventing changes from being made and removed Needs contributor approval labels Aug 27, 2023
Comment on lines 62 to 93
typedef enum {
/* 0 */ IBTN_BC_B,
/* 1 */ IBTN_BC_C_LEFT,
/* 2 */ IBTN_BC_C_DOWN,
/* 3 */ IBTN_BC_C_RIGHT,
/* 4 */ IBTN_BC_MAX
} InteractButtonBC;

#define IBTN_BC_C_FIRST IBTN_BC_C_LEFT
#define IBTN_BC_C_LAST IBTN_BC_C_RIGHT

typedef enum {
/* 0 */ IBTN_C_C_LEFT,
/* 1 */ IBTN_C_C_DOWN,
/* 2 */ IBTN_C_C_RIGHT,
/* 3 */ IBTN_C_MAX
} InteractButtonC;

#define IBTN_C_TO_BC(btnsC) ((btnsC) + 1)
#define IBTN_BC_TO_C(btnsBC) ((btnsBC) - 1)

typedef enum {
/* 0 */ IBTN_BCA_B,
/* 1 */ IBTN_BCA_C_LEFT,
/* 2 */ IBTN_BCA_C_DOWN,
/* 3 */ IBTN_BCA_C_RIGHT,
/* 4 */ IBTN_BCA_A,
/* 5 */ IBTN_BCA_MAX
} InteractButtonBCA;

#define IBTN_C_TO_BCA(btnsC) ((btnsC) + 1)
#define IBTN_BC_TO_BCA(btnsBC) (btnsBC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's too many abbreviations compact in a small place that throws me off for this. I'd have no issues fully spelling out INTERACT_BTN. What about:

typedef enum {
    /* 0 */ INTERACT_BC_BTN_B,
    /* 1 */ INTERACT_BC_BTN_C_LEFT,
    /* 2 */ INTERACT_BC_BTN_C_DOWN,
    /* 3 */ INTERACT_BC_BTN_C_RIGHT,
    /* 4 */ INTERACT_BC_BTN_MAX
} InteractBCButton;

#define INTERACT_BC_BTN_C_FIRST INTERACT_BC_BTN_C_LEFT
#define INTERACT_BC_BTN_C_LAST INTERACT_BC_BTN_C_RIGHT

typedef enum {
    /* 0 */ INTERACT_C_BTN_C_LEFT,
    /* 1 */ INTERACT_C_BTN_C_DOWN,
    /* 2 */ INTERACT_C_BTN_C_RIGHT,
    /* 3 */ INTERACT_C_BTN_MAX
} InteractCButton;

#define INTERACT_C_BTN_TO_BC_BTN(btnsC) ((btnsC) + 1)
#define INTERACT_BC_BTN_TO_C_BTN(btnsBC) ((btnsBC) - 1)

typedef enum {
    /* 0 */ INTERACT_BCA_BTN_B,
    /* 1 */ INTERACT_BCA_BTN_C_LEFT,
    /* 2 */ INTERACT_BCA_BTN_C_DOWN,
    /* 3 */ INTERACT_BCA_BTN_C_RIGHT,
    /* 4 */ INTERACT_BCA_BTN_A,
    /* 5 */ INTERACT_BCA_BTN_MAX
} InteractBCAButton;

#define INTERACT_C_BTN_TO_BCA_BTN(btnsC) ((btnsC) + 1)
#define INTERACT_BC_BTN_TO_BCA_BTN(btnsBC) (btnsBC)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this yeah, and I think it works with "interact"

the thing with "interact" is "there are other buttons you use to 'interact'", but this way e.g. INTERACT_C_BTN_ could read as "the interact C button(s)"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead with this at least for now

@Dragorn421
Copy link
Collaborator Author

@Dragorn421
Copy link
Collaborator Author

todo: look at macroify every access to the "interact btn"-indexed arrays and use a single enum

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs discussion There is a debate or other required discussion preventing changes from being made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants